Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite the conversion traits to work more like From and Into #176

Merged
merged 1 commit into from
Apr 5, 2020

Conversation

Ogeon
Copy link
Owner

@Ogeon Ogeon commented Mar 28, 2020

This replaces the old FromColor and IntoColor traits, as well as the implementations of From and Into between color types, with new traits that are more flexible. The new FromColor and IntoColor traits will also clamp the output to prevent out-of-bounds results.

It's a very breaking change. To migrate from the old system, any failing uses from and into should be replaced with from_color and into_color. Any custom color that used to implement IntoColor should switch to implementing FromColorUnclamped.

A new trait, called Transparency, has also been added to make it possible to implement FromColorUnclamped for Alpha without running into conflicting implementations. It makes it possible to convert from any color type that implements Transparency, and not just Alpha<A, T> to Alpha<B, T>, for example.

The semantics of the new traits are a bit different, so it may affect type inference. It may also be necessary to convert using more than one step. This is mostly for the RGB family and Luma, when changing RGB space or standard.

The syntax for the helper attributes were also changed to the more common #[palette(...)] pattern. It helped when implementing them and looks a bit nicer, IMO.

Closes #41, fixes #111.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 28, 2020

It's finally here, after all this time. I don't know how many times I rewrote the derives and how many versions of the traits I went through, but I'm quite happy with the current result. I was also helped a lot by more recent language features, such as not having to implement Into* for the "foreign" types.

I'm leaving it up for a while, in case anyone want to help reviewing. I may very well have forgotten something along the way.

type WithAlpha: Transparency<A, Color = Self::Color, WithAlpha = Self::WithAlpha>;

/// Transforms the color into a transparent color with the provided
/// alpha value. If `Self` already has a transparency, that one is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// alpha value. If `Self` already has a transparency, that one is
/// alpha value. If `Self` already has a transparency, that value is

I think the instances of "that one" in this doc section would be better as "that value" or "that field". I try to generally avoid pronouns and vague words like "it" and "one" in tech writing when possible.

/// color type. The type itself doesn't need to store the transparency
/// value, as long as it can be transformed into or wrapped in a type that
/// does have a representation of transparency. This would typically be done
/// by wrapping it in an [`Alpha`](struct.Alpha.html) instance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// `Transparency` is an interface for adding, removing and setting the alpha
/// channel of a color type. The type itself doesn't need to store the
/// transparency value as it can be transformed into or wrapped in a type that
/// has a representation of transparency. This would typically be done by
/// wrapping it in an [`Alpha`](struct.Alpha.html) instance.

fn with_alpha(self, alpha: A) -> Self::WithAlpha;

/// Removes the transparency from the color. If `Self::Color` has
/// an internal transparency field, that one should be set to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// an internal transparency field, that one should be set to
/// an internal transparency field, that field will be set to

similar change in L119

/// ```
fn without_alpha(self) -> Self::Color;

/// Removes the color into separate color and transparency values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Removes the color into separate color and transparency values.
/// Splits the color into separate color and transparency values.

//! * `rgb_space = "some::rgb_space::Type"`: Sets the RGB space
//! type that should be used when deriving. The default is to either use `Srgb`
//! or a best effort to convert between spaces, so sometimes it has to be set
//! to a specific type. This does also accept type parameters.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! to a specific type. This does also accept type parameters.
//! to a specific type. This also accepts type parameters.

/// such as allowing implementing `FromColor<Rgb<S2, T>> for Rgb<S1, T>`.
/// * Implementing `FromColorUnclamped` and `Limited` is enough to get all the other conversion
/// traits, while `From` and `Into` would not be possible to blanket implement in the same way.
/// This does also reduce the work that need to be done by macros.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// This does also reduce the work that need to be done by macros.
/// This also reduces the work that needs to be done by macros.

/// See the [`convert`](index.html) module for how to implement `FromColorUnclamped` for
/// custom colors.
pub trait FromColor<T>: Sized {
///Convert from T with values clamped to the color defined bounds.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
///Convert from T with values clamped to the color defined bounds.
/// Convert from T with values clamped to the color defined bounds.

/// See the [`convert`](index.html) module for how to implement `FromColorUnclamped` for
/// custom colors.
pub trait FromColorUnclamped<T>: Sized {
///Convert from T. The resulting color might be invalid in its color space.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
///Convert from T. The resulting color might be invalid in its color space.
/// Convert from T. The resulting color might be invalid in its color space.

@okaneco
Copy link
Contributor

okaneco commented Mar 30, 2020

This looks great 🎉

I don't have much to add other than small doc suggestions.

Since the file's being modified, the README's description section up top can probably be shortened to something like A Rust library that makes linear color calculations and color space conversion easy and accessible for anyone. since there's not a general color type anymore?

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 30, 2020

Thank you for taking a look at this! Those are good suggestions and feedback. I value any improvements to my writing, especially since English isn't my first language. I'll implement the suggestions and a couple of other improvements. I somehow forgot that "lossless" is a word.

@Ogeon
Copy link
Owner Author

Ogeon commented Apr 4, 2020

I applied the suggested documentation changes (and a few more) and rebased to get the latest changes from master.

@Ogeon
Copy link
Owner Author

Ogeon commented Apr 5, 2020

I decided to rename the Transparency to WithAlpha to keep the terminology more consistent. I was on the fence at first, but everything in and around it uses "alpha", so in the end I thought it would be weirder not to. This covers the last thing I wanted to change before squash+merge.

@Ogeon
Copy link
Owner Author

Ogeon commented Apr 5, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 5, 2020

Build succeeded

@bors bors bot merged commit dd7645d into master Apr 5, 2020
@bors bors bot deleted the conversion_traits branch April 5, 2020 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants